fix(desktop): enforce owner-only access in internal builds - #4053
Conversation
f7af909 to
16b66e5
Compare
|
Commenting on Wes Billman's behalf after a release-safety review of I found two blockers before this is safe to merge for an internal release:
The local spawn clamp, independent |
|
@wesbillman those make sense but don't seem like they should be blocking. Do you agree? |
16b66e5 to
825de5a
Compare
Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz> Co-authored-by: Amp <amp@ampcode.com> Ai-assisted: true
Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
825de5a to
f176e2d
Compare
Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
The merge with main pushed desktop/src/features/agents/hooks.ts past the 1000-line file-size ratchet. Move the owner-only access query into a dedicated module, which keeps hooks.ts under the limit and matches the existing one-hook-per-file pattern next to it. Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Commenting on Wes Billman's behalf after reviewing current head 99685b52b7daa7e385eca3db84c8fb916c709de1.
Two authorization/release blockers remain:
-
Existing provider deployments are not brought under the new policy. The owner-only projection is applied only while constructing a provider deploy payload (
desktop/src-tauri/src/commands/agents_deploy.rs:144-176), and that payload reaches the provider only through create-with-deploy or an explicit Start/deploy operation (desktop/src-tauri/src/commands/agents.rs:1001-1018,1125-1156). Existing remote infrastructure deliberately remains represented as deployed across Desktop restarts wheneverbackend_agent_idexists (desktop/src-tauri/src/managed_agents/runtime.rs:147-168). Upgrading to a marked build therefore leaves a previously deployedanyone/allowlist agent remotely open while the UI locks and displays “Only me.” That contradicts the PR's claim that owner-only access becomes effective for every managed agent in internal builds.Please reconcile already-deployed provider agents during rollout, or expose and enforce a fail-closed state until the provider has accepted an owner-only redeploy. Add an upgrade-path test covering an existing provider deployment with wider effective access.
-
owner-onlyis not literally owner-only. The harness admits the human owner and every cryptographically verified same-owner sibling agent (crates/buzz-acp/src/lib.rs:220-256). This branch explicitly relies on that broader set for Welcome teammates (desktop/src/features/onboarding/welcomeGuide.ts:264-275), while the UI says “Only me” and “Only you can send instructions” (desktop/src/features/agents/ui/RespondToField.tsx:69-76,237-240). Please either obtain and document the product/security decision that the intended boundary isowner ∪ verified same-owner agentsand make the user-facing contract accurate, or enforce literal human-owner-only access.
The local spawn clamp, independent BUZZ_ACP_ALLOWED_RESPOND_TO=owner-only guard, provider payload projection for new/explicit deployments, build-marker wiring, and OSS behavior otherwise look sound. CI is green at this head; I relied on the existing full Desktop checks rather than rerunning equivalent suites locally.
|
Additional blocker found in the independent security pass, commenting on Wes Billman's behalf:
The expanded |
|
One additional upgrade-path bug from Mongo's independent lifecycle review, confirmed at
This is not another authorization bypass—the Rust boundary still clamps effective access—but it is a real upgrade regression and contradicts the PR's goal of avoiding unnecessary Welcome restarts. Make the remediation target |
`provisionWelcomeTeam` decided whether a teammate needed an access write with `welcomeTeammateHasExpectedAccess`, which requires `owner-only` and an empty allowlist under the owner-only-access build policy, but then wrote `allowlist:[lead]` unconditionally. An upgraded install with pre-existing allowlisted teammates therefore rewrote state the predicate rejects on every provisioning pass, and `welcomeTeammateNeedsRestart` kept restarting teammates that were already running. Extract `welcomeTeammateAccessUpdate`, which returns the write that satisfies the predicate for the current build (or null when the teammate is already correct), and use it for the decision and the write so the two cannot drift again. Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
The baked `BUZZ_BUILD_AGENT_ENV` pairs are written into a spawned agent's environment last, after Desktop sets the access gates and identity vars, and nothing filtered them. A build packaged with `BUZZ_ACP_RESPOND_TO` or `BUZZ_ACP_ALLOWED_RESPOND_TO` set to `anyone` therefore produced agents that answer anyone while the UI shows the access locked to "Only me", so the build capability could disable its own enforcement. Reject reserved keys where the value enters the system, in `build.rs`, so such a build fails instead of shipping, and keep a runtime filter in `baked_build_env()` for a binary built without that check. The key list is `include!`d from one source into both consumers, matching the existing `reconnect_hook_config.rs` pattern, because a build script cannot import from the crate and the two copies must not drift. Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
Record what the owner-only-access build capability enforces (local spawn and every deploy payload this build serializes) and what it does not: a provider deployment created by an unmarked build keeps its wider remote access until it is next deployed from a marked build, even though this build's UI shows that agent locked to "Only me". Also state that the harness gate admits the owner and every verified same-owner sibling agent, which is the intended boundary and what the built-in Welcome team depends on, so a later reader does not mistake it for a hole in the clamp. Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz> Co-authored-by: Amp <amp@ampcode.com> Ai-assisted: true
Problem
Managed agents in internal Buzz builds should answer only their owner. Previously, an agent could keep a broader access setting and respond to other people, which did not match the access policy for internal builds.
This PR makes owner-only access effective for every managed agent in internal builds and makes that restriction clear in the Desktop UI. Open source builds remain configurable.
Changes
The companion #4064 explains the restriction in-thread when someone without access mentions an agent.
The enforcement will remain inactive in shipped builds until squareup/buzz-releases#74 marks internal releases during the build.
Screenshots
Tests
Added coverage for:
The full Desktop Rust and JavaScript suites, type checks, formatting, clippy, and file-size checks passed. Playwright E2E was not run.
Originated from Buzz channel buzz-agent-control. Supersedes #2537.